Added a message of enabled logs, and their paths.#13577
Conversation
|
@dotnet-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
Pull request overview
This PR improves MSBuild’s user-facing logging by surfacing which loggers are enabled (e.g., /bl, /fl) and where their output files are being written, addressing confusion called out in #12261.
Changes:
- Introduces
LoggerRegisteredEventArgsto publish information about enabled loggers (name/verbosity/output paths). - Adds
IFileOutputLoggerand updatesBinaryLogger/FileLoggerto expose their output file paths. - Updates TerminalLogger/ParallelConsoleLogger to collect these events and print output paths at build end; adds new localized resource strings and unit tests.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Framework/LoggerRegisteredEventArgs.cs | New Framework event args type used to report enabled logger details |
| src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs | Emits “enabled logs” message + logger registration events during build start |
| src/Build/Logging/IFileOutputLogger.cs | New internal interface for loggers that produce a primary output file |
| src/Build/Logging/FileLogger.cs | Implements IFileOutputLogger to expose the log file path |
| src/Build/Logging/BinaryLogger/BinaryLogger.cs | Implements IFileOutputLogger to expose the binlog path |
| src/Build/Logging/TerminalLogger/TerminalLogger.cs | Captures logger-registration events and prints clickable paths in summary |
| src/Build/Logging/ParallelLogger/ParallelConsoleLogger.cs | Captures logger-registration events and prints paths in summary |
| src/Build/Resources/Strings.resx | Adds new resource strings for enabled logs / output file paths |
| src/Build/Resources/xlf/Strings.*.xlf | Adds corresponding localization entries (state=new) |
| src/Build/Microsoft.Build.csproj | Includes the new IFileOutputLogger.cs compile item |
| src/Build.UnitTests/BackEnd/LoggingServicesLogMethod_Tests.cs | Adds unit tests for enabled log names and file path reporting |
|
@AlesProkop can you add to the description screenshots/log examples how it looks like with various configs? I'm interersted in the matrix: (console / terminallogger) x verbosity x (/bl or /fl) |
@JanProvaznik |
| { | ||
| internal interface IFileOutputLogger | ||
| { | ||
| System.Collections.Generic.IReadOnlyList<string> OutputFilePaths { get; } |
There was a problem hiding this comment.
This needs documentation/scoping - it'd be entirely reasonable for a logger author to think that this represented the per-project-build primary outputs, so we need comments or naming to clarify the intent/scope of this member.
There was a problem hiding this comment.
I wonder if this should be more of a generic string "summary message". For file loggers it makes sense to specify paths, but for example in the once-common path of -flp1:ErrorsOnly;LogFile=build.err -flp2:WarningsOnly;LogFile=build.wrn -flp3:Verbosity=normal;LogFile=build.log the loggers could emit summaries like
Error log written to build.err
Warning log written to build.wrn
Log at "normal" verbosity written to build.log
?
I'm not sure we actually need that, though.
There was a problem hiding this comment.
I added documentation for now. If you think the more generic summary message is worth pursuing in this PR, I will implement it.
There was a problem hiding this comment.
@baronfel can you share your opinion whether we would like to have a more generic summary controlled by the logger?
There was a problem hiding this comment.
This is an internal interface so I think this is fine for now. If it were public we'd have to think more about what the external surface area would be.
rainersigwald
left a comment
There was a problem hiding this comment.
Overall I like the approach.
| { | ||
| internal interface IFileOutputLogger | ||
| { | ||
| System.Collections.Generic.IReadOnlyList<string> OutputFilePaths { get; } |
There was a problem hiding this comment.
I wonder if this should be more of a generic string "summary message". For file loggers it makes sense to specify paths, but for example in the once-common path of -flp1:ErrorsOnly;LogFile=build.err -flp2:WarningsOnly;LogFile=build.wrn -flp3:Verbosity=normal;LogFile=build.log the loggers could emit summaries like
Error log written to build.err
Warning log written to build.wrn
Log at "normal" verbosity written to build.log
?
I'm not sure we actually need that, though.
| CultureInfo.CurrentCulture.TextInfo.ListSeparator + " ", | ||
| logger.OutputFilePaths.Select(outputPath => $"{AnsiCodes.LinkPrefix}{new Uri(outputPath).AbsoluteUri}{AnsiCodes.LinkInfix}{outputPath}{AnsiCodes.LinkSuffix}")); | ||
|
|
||
| Terminal.WriteLine(ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("LogFileOutputPath", logger.LoggerName, displayPaths)); |
There was a problem hiding this comment.
This should be under a changewave, in case it breaks someone and we need to change it to be more opt-in.
There was a problem hiding this comment.
I think I did it? I need a review if I did it correctly as it is my first time working with that.
Fixes #12261
Context
There was no clear message in the logs when using either the
/blor the/flflag that would indicate the location of the files that are being created by these logs. Also, there was no consistent way of telling which logs were enabled.Changes Made
LoggingServiceLogMethods.cs
Added a method LogLoggersUsed(bool fileNames) which lists the names of all enabled logs in a diagnostic verbosity. Added new EventArgs which registers every enabled logger with information. This is then used for logging clickable links of paths that were created by loggers.
LoggerRegisteredEventArgs.cs
New EventArgs used for registering enabled loggers. It cointains infromation about the logger, such as the name of the logger, verbosity on which the logger operates on, file paths to files created by the logger and possible additional output file paths.
ParallelConsoleLogger.cs
Added handling of the LoggerRegisteredEventArgs.
TerminalLogger.cs
Added handling of the LoggerRegisteredEventArgs.
FileLogger.cs
Added a way to read the path to the file created by the filelogger, similarly to how binlog operates.
Testing
Added unit tests to LoggingServicesLogMethod_Tests.cs.
Examples
BinaryLogger, FileLogger, ConsoleLogger
In the console log, we have clickable links:
In the file log, we have
Currently, the BinLogger logs its path in its own way but we can see the enabled logs (this will be the same if we enable higher verbosity in all of the logs.)
Enabled logs: BinaryLogger, CentralForwardingLogger, CentralForwardingLogger, ConsoleLogger, FileLogger, InternalTelemetryConsumingLogger, InternalTelemetryForwardingLoggerThen
BinaryLogger, FileLogger, TerminalLogger
In the terminal log,
/tlp:summaryhas to be enabled, otherwise, it will not show (this is consistent with how other messages are handled in the TerminalLogger)